Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

part 1 of romfs extend to enable format function #13445

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

guohao15
Copy link
Contributor

Summary

part 1 of romfs extend to enable format function

Impact

This is part 1 of a series of patchs.

To enhance the write capabilities of ROMFS, enabling it to perform simple file creation, writing, and deletion operations while maintaining compatibility with the original ROMFS format used in open-source ROMFS.

Feature is enabled by CONFIG_FS_ROMFS_WRITEABLE Kconfig
When CONFIG_FS_ROMFS_WRITEABLE is enabled
We can use -o rw to enable writable

Testing

cmoka_fs_test
cmoka_syscall_test

guohao15 and others added 7 commits September 13, 2024 11:49
change romfs_cachenode to find all headers

Signed-off-by: guohao15 <[email protected]>
add sparelist api

Signed-off-by: guohao15 <[email protected]>
fix for df cmd infomation of romfs

Signed-off-by: guohao15 <[email protected]>
origoffset represent the offset of current node
offset represent the offset of the file when
the current node is a soft or hard link

Signed-off-by: guohao15 <[email protected]>
…ing an MTD driver interface using the standard mount() operation."

This reverts commit 5708a1a.

Signed-off-by: dongjiuzhu1 <[email protected]>
add romfs_mkfs to support autoformat && forceformat

Signed-off-by: guohao15 <[email protected]>
@lupyuen
Copy link
Member

lupyuen commented Sep 18, 2024

[Experimental Bot, please feedback here]

The PR summary and impact descriptions provide a good start, but lack some specifics to fully meet NuttX requirements. Here's a breakdown and suggestions for improvement:

Summary - Needs Improvement:

  • Why change is necessary? You mention enhancing write capabilities, but not the underlying reason. Is this a user request? Does it address a specific limitation or bug? Be explicit about the "why".
  • What functional part is changed? "romfs extend" is vague. Mention specific files/modules within the ROMFS subsystem that are modified.
  • How does the change work? "enable format function" is the result, not the mechanism. Describe at a high-level the technical approach taken (e.g., new data structures, modifications to existing algorithms, etc.).

Impact - Mostly Good:

  • New feature added? Clearly stated, but briefly elaborate on the capabilities this new feature provides.
  • Impact on user? Mentioning CONFIG_FS_ROMFS_WRITEABLE and -o rw is good, but provide a short example of how a user would utilize this in practice.
  • Other impacts: While you mark most as "NO", a multi-part change like this likely has implications. Even if unchanged now, consider:
    • Build: Will later parts require build system changes? If unsure, acknowledge this.
    • Documentation: Part 1 likely needs documentation updates. If not included now, mention it's planned for a later part.

Testing - Needs Significant Improvement:

  • Build Hosts & Targets: Provide specifics, don't leave it generic. List OS, versions (compiler especially), and for targets the exact board:config combinations used.
  • Testing Logs: "cmoka_fs_test" is not useful. Include relevant snippets of logs demonstrating:
    • The issue BEFORE your change (proving it was a problem)
    • Successful operation AFTER the change
    • Ideally, test cases covering both normal usage AND error handling

Overall:

The PR is on the right track but needs more detail and concrete evidence of testing. Addressing the points above will make it much stronger.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants